GH-36311: [C++] Fix integer overflows in `utf8_slice_codeunits` #36575

benibus · 2023-07-09T04:32:46Z

Rationale for this change

The default value for the SliceOptions::stop is INT64_MAX, which isn't considered in several internal calculations - resulting in integer overflows and unexpected behavior when stop isn't provided.

Also note that running the included tests without the fixes should result in ubsan errors (it did for me, at least).

What changes are included in this PR?

Adds some logic to SliceCodunitsTransform that handles potential overflows
Adds tests for cases where the start param is positive/negative and stop is the maximum value

Update
Discovered that utf8_slice_codeunits deviates from Python array behavior when stop=None and step < 0, so further changes were made:

Handles INT64_MIN for SliceOptions::stop on C++ side, adds more tests.
Updates Python bindings for SliceOptions so that the default value when stop=None (sys.maxsize) is negated when step < 0
Adds None as a possible stop value in Python tests

Are these changes tested?

Yes (tests are included)

Are there any user-facing changes?

In theory, altering the behavior of utf8_slice_codepoints when stop=None and step < 0 could be considered a breaking change. That being said, the current implementation produces incorrect results whenever None is even used, so it probably isn't one in practice...

Closes: [C++][Python] utf8_slice_codeunits produces invalid unicode sequence #36311

benibus · 2023-07-09T17:28:57Z

@pitrou I haven't added any python tests (although I can if necessary) - but the original examples should be working now as well.

>>> pa.compute.utf8_slice_codeunits(f"AB{chr(127917)}C{chr(127917)}ㇱD", start=2, stop=None, step=4)
<pyarrow.StringScalar: '🎭D'>

>>> s = f"AB{chr(127917)}C{chr(127917)}ㇱD"
>>> a = pa.array([s])
>>> pc.utf8_slice_codeunits(a, start=0, stop=None)
<pyarrow.lib.StringArray object at 0x7f0fa4bb85e0>
[
  "AB🎭C🎭ㇱD"
]

pitrou · 2023-07-10T07:40:54Z

The current Python tests already exercise the utf8_slice_codeunits function against Python slicing, so it would be nice to expand them a bit to also cover the case where stop is unbounded:

arrow/python/pyarrow/tests/test_compute.py

Line 537 in a63ead7

def test_slice_compatibility():

pitrou · 2023-07-10T09:33:35Z

This PR is almost ready for merging and would be nice to include in 13.0.0. @raulcd

pitrou · 2023-07-10T09:33:47Z

@github-actions crossbow submit -g cpp

benibus · 2023-07-10T17:44:07Z

This may need more work, unfortunately. I've learned that passing stop=None doesn't produce the same results as it would for python arrays if step is also negative. For instance:

>>> s = pa.scalar("𝑓öõḍš")
>>>
>>> pc.utf8_slice_codeunits(s, start=2, stop=None, step=-1)
<pyarrow.StringScalar: ''>
>>>
>>> s.as_py()[2:None:-1]
'õö𝑓'

The current behavior is actually equivalent to this:

>>> s.as_py()[2:sys.maxsize:-1]
''

So yeah... expanding the python tests was a good call.

Also reorganizes C++ tests

When `stop=None`, negates the default value of `sys.maxsize` if `step < 0` to mimic the behavior of Python arrays

pitrou · 2023-07-10T20:35:28Z

@github-actions crossbow submit -g cpp

pitrou · 2023-07-10T21:30:16Z

@github-actions crossbow submit -g cpp

github-actions · 2023-07-10T21:33:06Z

Revision: 58eb873

Submitted crossbow builds: ursacomputing/crossbow @ actions-4f1abc3b6c

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-35-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20

wirable23 · 2023-07-11T04:32:55Z

Thank you for fixing this @benibus! I think this also fixes few other open issues:

#34917
#34928
#14991
#34929

pitrou · 2023-07-11T06:33:16Z

@raulcd It would be nice to cherry-pick this in 13.0.0.

### Rationale for this change The default value for the `SliceOptions::stop` is `INT64_MAX`, which isn't considered in several internal calculations - resulting in integer overflows and unexpected behavior when `stop` isn't provided. Also note that running the included tests without the fixes should result in ubsan errors (it did for me, at least). ### What changes are included in this PR? - Adds some logic to `SliceCodunitsTransform` that handles potential overflows - Adds tests for cases where the `start` param is positive/negative and `stop` is the maximum value **Update** Discovered that `utf8_slice_codeunits` deviates from Python array behavior when `stop=None` and `step < 0`, so further changes were made: - Handles `INT64_MIN` for `SliceOptions::stop` on C++ side, adds more tests. - Updates Python bindings for `SliceOptions` so that the default value when `stop=None` (`sys.maxsize`) is negated when `step < 0` - Adds `None` as a possible `stop` value in Python tests ### Are these changes tested? Yes (tests are included) ### Are there any user-facing changes? In theory, altering the behavior of `utf8_slice_codepoints` when `stop=None` and `step < 0` could be considered a breaking change. That being said, the current implementation produces incorrect results whenever `None` is even used, so it probably isn't one in practice... * Closes: #36311 Authored-by: benibus <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

conbench-apache-arrow · 2023-07-21T19:40:01Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 143c691.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

rachtsingh · 2023-10-24T18:00:13Z

It would also be nice to backport this to 12.0.0 if possible, since right now we can't upgrade to 13.0.0 (see pandas-dev/pandas#55374 (comment)).

Avoid overflows in SliceCodunitsTransform

3a5f286

github-actions bot added Component: C++ awaiting review Awaiting review labels Jul 9, 2023

benibus marked this pull request as ready for review July 9, 2023 17:25

This comment was marked as outdated.

Sign in to view

benibus added 3 commits July 10, 2023 15:45

Handle INT64_MIN in SliceCodeunitsTransform

40a7afc

Also reorganizes C++ tests

Update Python bindings for SliceOptions

f6e013e

When `stop=None`, negates the default value of `sys.maxsize` if `step < 0` to mimic the behavior of Python arrays

Test stop=None for Python

03fa729

benibus requested a review from AlenkaF as a code owner July 10, 2023 19:53

github-actions bot added the Component: Python label Jul 10, 2023

pitrou approved these changes Jul 10, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 10, 2023

This comment was marked as outdated.

Sign in to view

Fix negation UB

58eb873

pitrou merged commit 143c691 into apache:main Jul 11, 2023

pitrou removed the awaiting committer review Awaiting committer review label Jul 11, 2023

This was referenced Jul 11, 2023

[C++] utf8_slice_codeunits crashes with max start and negative step #34928

Closed

[Python] pyarrow.compute.utf8_slice_codeunits fails when stop=None #14991

Closed

mroeschke mentioned this pull request Dec 6, 2023

BUG: Arrow backed series can't be displayed after string slicing it pandas-dev/pandas#53833

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-36311: [C++] Fix integer overflows in `utf8_slice_codeunits` #36575

GH-36311: [C++] Fix integer overflows in `utf8_slice_codeunits` #36575

benibus commented Jul 9, 2023 •

edited

Loading

benibus commented Jul 9, 2023

pitrou commented Jul 10, 2023

pitrou commented Jul 10, 2023

pitrou commented Jul 10, 2023

This comment was marked as outdated.

benibus commented Jul 10, 2023

pitrou commented Jul 10, 2023

This comment was marked as outdated.

pitrou commented Jul 10, 2023

github-actions bot commented Jul 10, 2023

wirable23 commented Jul 11, 2023

pitrou commented Jul 11, 2023

conbench-apache-arrow bot commented Jul 21, 2023

rachtsingh commented Oct 24, 2023

GH-36311: [C++] Fix integer overflows in utf8_slice_codeunits #36575

GH-36311: [C++] Fix integer overflows in utf8_slice_codeunits #36575

Conversation

benibus commented Jul 9, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

benibus commented Jul 9, 2023

pitrou commented Jul 10, 2023

pitrou commented Jul 10, 2023

pitrou commented Jul 10, 2023

This comment was marked as outdated.

benibus commented Jul 10, 2023

pitrou commented Jul 10, 2023

This comment was marked as outdated.

pitrou commented Jul 10, 2023

github-actions bot commented Jul 10, 2023

wirable23 commented Jul 11, 2023

pitrou commented Jul 11, 2023

conbench-apache-arrow bot commented Jul 21, 2023

rachtsingh commented Oct 24, 2023

GH-36311: [C++] Fix integer overflows in `utf8_slice_codeunits` #36575

GH-36311: [C++] Fix integer overflows in `utf8_slice_codeunits` #36575

benibus commented Jul 9, 2023 •

edited

Loading